Skip to content

[MRG] Ensure check is passed to update function #1411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 16, 2025
Merged

[MRG] Ensure check is passed to update function #1411

merged 3 commits into from
Jun 16, 2025
+19 −5

Conversation

@teonbrooks
Copy link
Member

@teonbrooks teonbrooks commented Jun 4, 2025

PR Description

I noticed that when using the BIDSPath and having passed the argument check=False, the value wasn't being passed to the underlying update function.

I noticed this when I was passing the argument datatype='eyetrack' (not supported by BIDS validation) but with the check=False. when it came time to use BIDSPath.fpath, it errored

Within the update method, it does a self-assign to check, and since the check argument isn't passed, it defaults to True

In [1]: bids_path = BIDSPath(root=bids_root, session=None, task=task,
    ...:                      datatype='eyetrack', check=False)

In [2]: bids_path.fpath
...
ValueError: datatype (eyetrack) is not valid. Should be one of ['meg', 'eeg', 'ieeg', 'nirs', 'anat', 'beh']

In [3]: bids_path.check
Out[3]: False

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

@codecov
Copy link

codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (a619cb7) to head (b2253d3).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1411   +/-   ##
=======================================
  Coverage   97.45%   97.45%           
=======================================
  Files          40       40           
  Lines        9088     9093    +5     
=======================================
+ Hits         8857     8862    +5     
  Misses        231      231

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hoechenberger
Copy link
Member

hoechenberger commented Jun 5, 2025 •
edited
Loading

Hey Teon, good to see you again 👍 I hope things are going well for you.

I think we'll need both lines here – the one you removed and the one you added – to make things work as documented (docs say that update(check=...) will set the .check attribute)

Care to update?

@hoechenberger hoechenberger requested a review from sappelhoff June 5, 2025 13:43
hoechenberger
hoechenberger approved these changes Jun 5, 2025
View reviewed changes
Copy link
Member

@hoechenberger hoechenberger left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, disregard my previous comment, I think your proposed fix is exactly right. I got confused between the two methods!

@larsoner
Copy link
Member

larsoner commented Jun 5, 2025

Hi @teonbrooks 👋 !!!

... would be great to have a test that fails on main and passes on this PR 😄

@teonbrooks
Copy link
Member Author

teonbrooks commented Jun 5, 2025

so sorry, where are my manners?! hiya! 👋🏾 good to see you both 😄

@teonbrooks
Copy link
Member Author

teonbrooks commented Jun 5, 2025

... would be great to have a test that fails on main and passes on this PR 😄

cool! i'll get on that. I'll update the title to WIP as a reminder. I should have some time to get to it tomorrow (maybe today if I'm lucky)

@teonbrooks teonbrooks marked this pull request as draft June 5, 2025 16:39
@sappelhoff
Copy link
Member

sappelhoff commented Jun 7, 2025

Hi Teon 👋 thanks for taking care of this 🚀

teonbrooks and others added 2 commits June 15, 2025 15:03
@teonbrooks
Added test, Fix private function
50cbfd0 
also within `_get_matching_bidspaths_from_filesystem`, the check parameter was being passed so it would not work when trying to generate a fpath

Added test to check for the fpath creation with datatype not in VALID_DATATYPES when check=False
@pre-commit-ci @teonbrooks
[pre-commit.ci] auto fixes from pre-commit.com hooks
b2253d3 
for more information, see https://pre-commit.ci
@teonbrooks teonbrooks marked this pull request as ready for review June 15, 2025 19:04
@teonbrooks teonbrooks mentioned this pull request Jun 15, 2025
Open
@teonbrooks teonbrooks changed the title [BUG] Check isn't being passed to update function [MRG] Ensure check is passed to update function Jun 15, 2025
@teonbrooks
Copy link
Member Author

teonbrooks commented Jun 15, 2025

Got around to it this afternoon. If someone could give it a quick review, I would appreciate it :)

@larsoner larsoner merged commit cc0fc70 into mne-tools:main Jun 16, 2025
24 checks passed
@larsoner
Copy link
Member

larsoner commented Jun 16, 2025

Thanks @teonbrooks !

@teonbrooks teonbrooks deleted the dev-bugfix branch June 16, 2025 14:59
@sappelhoff sappelhoff mentioned this pull request Jun 17, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hoechenberger hoechenberger hoechenberger approved these changes

@sappelhoff sappelhoff Awaiting requested review from sappelhoff

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@teonbrooks @hoechenberger @larsoner @sappelhoff